Make stream deletion asynchronous, resumable across restarts - #1770

No actionable comments were generated in the recent review. 🎉

Walkthrough

Stream deletion now uses serialized create/delete operations, durable tombstones, asynchronous cleanup, deduplication, and recovery handling. Creation rejects active deletion. Local listing and migration handle interrupted deletion. Test compose files use MinIO from Quay.io.

Changes

Stream deletion lifecycle

MinIO test image sources

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant DeleteHandler
  participant StreamStore
  participant ObjectStorage
  participant SyncAndMigration
  Client->>DeleteHandler: delete stream
  DeleteHandler->>StreamStore: mark deleting under CREATE_STREAM_LOCK
  DeleteHandler->>ObjectStorage: write tombstone
  DeleteHandler->>ObjectStorage: schedule deduplicated deletion
  DeleteHandler-->>Client: return deletion started
  SyncAndMigration->>ObjectStorage: list tombstoned streams
  SyncAndMigration->>ObjectStorage: resume deletion and clear tombstone

Merge Risk: ⚪ Minimal · up to 4403a

Deletion cleanup retries transient tombstone-clear failures, and asynchronous ingestor deletion reports the accepted status correctly. No actionable merge-blocking risk remains.

• Fix all pre-merge checks with AI

_{A rabbit reads each line,
The patch grows clear beneath the moon,
Small changes hop in place,
Tests guard the garden path,
Reviews bloom before the dawn.}

_{Comment @coderabbitai help to get the list of available commands.}

CI status update: the two Quest integration test failures (Distributed and Standalone) are an expected consequence of this PR's intentional API contract change, not a bug in the implementation.

Root causes, confirmed from the CI logs:

• DELETE /logstream/{stream} now returns 202 Accepted instead of 200 OK (by design, since the deletion is now asynchronous). Every Quest test whose setup/teardown does "delete stream, assert 200" fails at that assertion and then runs in a partially-cleaned-up state, which cascades into several unrelated-looking failures later in the same sequential test run.
• Recreating a stream immediately after deleting it can now correctly return 409 Conflict ("being deleted, please retry shortly") instead of silently succeeding, since the old stream may still be mid-deletion. A few tests that delete-then-immediately-recreate a stream with the same name hit this.

Both are already called out under "API contract changes" in the PR description. Quest (quay.io/parseablehq/quest:main) is a separate repo/image and will need its assertions updated to expect 202 for stream deletion and to tolerate/retry on a transient 409 when recreating a stream right after deleting it, before this PR's CI can go green.

Opened a companion fix for the Quest test suite: parseablehq/quest#126 (updates the hardcoded 200 assertions to 202, and adds retry-on-409 for tests that recreate a stream right after deleting it).

How the tombstone-based deletion works

The core problem: today, DELETE /logstream/{stream} blocks the HTTP response on a full recursive object-store delete. For a TB-scale stream that's potentially millions of keys, so the client waits minutes for something that should be instant.

The fix, in three parts:

1. Durable marker, placed outside the stream's own prefix. When a delete request comes in, we write a tiny marker object to .tombstones/{tenant}/{stream_name}/marker — deliberately not under the stream's own {tenant}/{stream_name}/... prefix. That matters because the actual bulk delete is a single recursive LIST-then-DELETE over that exact prefix. If the tombstone lived inside it, the bulk delete could sweep it up mid-job, and then a crash right after would leave no record that a deletion was ever in progress — breaking the "resume on restart" guarantee. Placing it outside makes it structurally immune to that, regardless of backend or listing order.

2. In-memory deleting flag, set before the tombstone write. The moment a delete request lands, we flip a deleting bool on the resident Stream object in memory (not persisted — it's re-derived from the tombstone on reload). This flag is checked at every place a stream could be touched: query execution, ingestion, schema/stats lookups, and stream reload from storage. It's set before the tombstone put_object call completes, with no await in between, specifically to close a race where a concurrent request on the same node could otherwise slip through in the gap between "tombstone durable" and "flag set."

3. Respond immediately, delete in the background. Once the tombstone is written and the flag is set, we respond 202 Accepted right away. The actual bulk delete runs in a spawned background task. When it finishes, it clears the tombstone and removes the stream from memory. If the node crashes mid-delete, the tombstone survives (per point 1), and on restart we scan .tombstones/ and resume the deletion for anything still marked — so there's no orphaned half-deleted stream state after a crash.

Cross-node correctness (this is a distributed system, not a single process):

• The node that receives the DELETE (query node, or standalone) is the only one that ever runs the actual physical bulk delete.
• It notifies all live ingestors synchronously (before responding) to flag the stream deleting locally and stop new writes to it.
• As a fallback for an ingestor that was down or partitioned when that notification went out, the periodic sync job also checks for the tombstone and self-heals — so the worst case is bounded to one sync interval, not indefinite staleness.
• Only the originating node's background job ever runs the actual delete-stream call — ingestors just flag and wait, they never independently re-trigger the bulk delete.

Split into two PRs:

• Add tombstone/deleting safety net for stream deletion (#1763) #1768 is pure scaffolding — tombstone helpers, the deleting flag, and guards everywhere that checks it — with zero change to today's delete handlers' actual behavior.
• Make stream deletion asynchronous, resumable across restarts #1770 is what wires it up: writes the tombstone, sets the flag, fans out to ingestors, spawns the background delete, and changes the response to 202.

Happy to expand on any specific part (crash-recovery ordering, ingestor self-heal timing, why 202 vs 200, etc.) if useful.

@prabhaks i have merged the previous PR #1768 can you resolve the conflict in this PR and make it ready for review

@prabhaks i have merged the previous PR #1768 can you resolve the conflict in this PR and make it ready for review

@nikhilsinhaparseable Can you help merge this PR: parseablehq/quest#126 which is dependency for this one! WIthout the quest pr, the integration tests with quest fails here!

Once the CI succeeds, I will mark this PR for review!

Pushed a follow-up commit addressing this round of review feedback:

• clear_deleting() rolls back mark_deleting() when the tombstone write itself fails, so a transient storage error doesn't permanently strand a stream in "deleting" state.
• Reordered the standalone and query-node delete handlers so spawn_stream_deletion runs last, after local dir and hot-tier cleanup, and made the ingestor fan-out and hot-tier cleanup best-effort (log and continue) instead of failing the whole request.
• create_update_stream now rejects create/update of a tombstoned-but-not-yet-purged stream even when it isn't resident in memory on that node yet, not just when it's already loaded.
• LocalFS's delete_stream now treats a repeated delete on an already-empty prefix as success, matching S3/Azure/GCS behavior, instead of erroring on retry.
• sync_all_streams's tombstone check now only reaps the resident entry on non-owner nodes, closing a race where the owning node's own in-flight tombstone write could get its deleting flag wiped by the periodic sync.
• Fixed a stale comment on the dedup test module that no longer matched the real entry()-based atomic guard.

Separately, on the CI failure: the actual failure wasn't the quest assertion mismatch the job name suggested, it was docker compose up --build failing to pull minio/minio at all (pull access denied). MinIO has moved off Docker Hub. Switched all four docker-compose test files to quay.io/minio/minio with the same pinned release tag. I don't have Docker available in this environment so I couldn't verify the pull/compose-up locally, but I confirmed via the Quay API that this exact tag exists and resolves there.

The Quest CI checks will still need parseablehq/quest#126 merged before they can pass end to end.

Dug into the CI failure and the two newest CodeRabbit findings.

Distributed Quest failure, root cause found from the raw logs (not the assertion mismatch quest#126 fixes):

Right after a stream was deleted and recreated under the same name (a pattern several Quest tests use), ingestion against it kept failing with 409 "is being deleted", even though the delete had long since finished. The querier itself recreated the stream fine, but the ingestor node never got the memo: an ingestor doesn't run the background deletion job, so it only clears its own is_deleting flag when sync_all_streams next ticks and notices the tombstone is gone. Recreating the stream before that tick landed the ingestor in a stuck state, and since the querier-to-ingestor stream sync is fire-and-forget, the failure was silent.

Fixed by having create_update_stream re-check the durable tombstone before trusting a resident is_deleting flag, and self-heal by clearing it immediately if the tombstone's already gone, instead of waiting on the next sync interval.

Two new CodeRabbit findings (both confirmed real):

• mark_deleting() + the tombstone write in a DELETE handler could interleave with a concurrent create_update_stream on the same node, since only the query node's put_stream held CREATE_STREAM_LOCK, and none of the DELETE handlers did. Standalone and ingestor put_stream/delete had no lock at all.
• Fixed by having all three delete handlers (standalone, query, ingest) and their corresponding put_stream handlers hold the same lock across the is_deleting-check-through-tombstone-write window. Cross-node atomicity (e.g. a delete on the query node racing a create forwarded to a different node) is a larger change and stays out of scope here, same as the existing best-effort fan-out.

Pushed as a follow-up commit. Full test suite (490 tests) still green, cargo fmt/clippy clean.

Found the real cause of the latest CI failures (both Quest suites failing, plus DeepSource).

The previous commit's self-heal for a stale is_deleting() flag cleared the flag but left stream_in_memory_dont_update (computed earlier in create_update_stream) unchanged. So right after self-healing, execution fell straight into the pre-existing "Logstream already exists" check and returned 400 instead of proceeding to actually create the stream. This is exactly what Quest's delete-then-recreate pattern (TestSmokeCreateStream deletes the shared stream, TestSmokeIngestEventsToStream immediately recreates it) hits every time:

Error: Not equal:
expected: 200
actual  : 400
Messages: Server returned http code: 400 Bad Request

Confirmed identically in both the standalone log and the distributed log (query node forwarding the same create to the ingestor hit it too: "Logstream ... already exists, please create a new log stream with unique name"), and it's the reason everything after TestSmokeIngestEventsToStream in the distributed run cascaded into failures as well (shared stream left in a bad state).

Fix: once the stale flag is confirmed stale (tombstone gone) and cleared, the stale in-memory entry is also dropped from the streams map (not just its flag), and stream_in_memory_dont_update is corrected to false. That way get_or_create doesn't hand back the same stale Arc<Stream>, and the "already exists" check no longer trips.

Also pulled the deletion/tombstone checks out of create_update_stream into a new reject_if_stream_deleting helper -- the added branches had pushed its cyclomatic complexity to "very-high" risk per DeepSource (RS-R1000), which is what caused the DeepSource check to fail.

Verified locally: cargo build --lib, cargo fmt --check, cargo clippy --lib --all-targets all clean, cargo test --lib 490 passed. Pushed as a new commit.

@coderabbitai review

@coderabbitai review

@coderabbitai review